feat(csharp/src/Drivers/Apache): increase telemetry instrumentation for Apache drivers #3794
Conversation
…or Apache drivers
|
@CurtHagenlocher - When you have some time, can you review this small PR to instrument more tracing in the other Thrift-based driver? Thanks. |
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! Today, we have a bunch of helpers that work with object-typed tag values, and many of the existing calls pass types other than strings. But in this change, almost everything is converted to a string first. I think we need to decide whether tag values should always be strings or whether they can be other object values as well and be consistent about the path we take.
| httpClient.DefaultRequestHeaders.AcceptEncoding.Add(new StringWithQualityHeaderValue("identity")); | ||
| httpClient.DefaultRequestHeaders.ExpectContinue = false; | ||
|
|
||
| activity?.AddTag(ActivityKeys.Encrypted, TlsOptions.IsTlsEnabled.ToString()); |
There was a problem hiding this comment.
We use object-valued tags in a lot of other places; why do the ToString() here?
Same comment applies to AddTag calls throughout this change.
There was a problem hiding this comment.
This has to do with a previous failed attempt to use JsonSerializerContext for the SerialializableActivity class.
When using JsonSerializerContext, it would throw and exception if it didn't know how to serialize a particular object that was added to a Tag (e.g., collection, etc.). This issue could be fixed by adding that class to the definition, but that would mean any instrumenters of tracing would have to remember to decorate this class.
So, I ended up removing it.
My intention is to simplify the AddTag so that only types that could be handled by JsonSerializerContext natively should be added.
I think I went overboard, as I believe numeric and boolean types should serialize fine.
At any rate, if we do restrict the types passed to AddTag, we can do that in one change rather than a trickle.
I'll remove the ToString() usage except for enums, where the name is better than than the number.
There was a problem hiding this comment.
I've remove unnecessary ToString() calls for boolean and URI. I've left them in for enum types, as it is easier to read.
3 participants
Adds more telemetry information for Apache drivers
Activity.AddTaginstead ofActivity.SetTagasSetTagis more expensive.